runtime/pprof: avoid panic for sigprofNonGoPC short stacks - #80890
runtime/pprof: avoid panic for sigprofNonGoPC short stacks#80890Super-long wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
This PR (HEAD: 0c0a01c) has been imported to Gerrit for code review. Please visit Gerrit at https://go-review.googlesource.com/c/go/+/815820. Important tips:
|
|
Message from Gopher Robot: Patch Set 1: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/815820. |
|
Message from Gopher Robot: Patch Set 1: Congratulations on opening your first change. Thank you for your contribution! Next steps: Most changes in the Go project go through a few rounds of revision. This can be During May-July and Nov-Jan the Go project is in a code freeze, during which Please don’t reply on this GitHub thread. Visit golang.org/cl/815820. |
|
Message from 李兆龙: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/815820. |
|
Message from Ian Lance Taylor: Patch Set 2: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/815820. |
sigprofNonGoPC produces a short stack {pc, _ExternalCode} when a
SIGPROF lands while m.isExtraInC is true. This happens in a race
window in cgocallbackg: when a C thread reuses an extra M (g != nil,
needm skipped), isExtraInC is still true from the previous callback
return, and is only cleared after exitsyscall returns. If that PC was
previously sampled in a normal Go context, appendLocsForStack has
cached an inlined expansion (l.pcs) longer than the short stack, and
panics with "stack too short to match cached location".
The panic fires in the runtime-spawned profileWriter goroutine,
which user code cannot recover. In -buildmode=c-shared binaries the
runtime forces tracebackCrash, so it becomes SIGABRT, crashing the
host process.
Fix: clear isExtraInC inside exitsyscall, right after the goroutine
transitions to _Grunning and before acquiring a P. This eliminates
the race window. For non-cgo syscall exits, isExtraInC is already
false, so this is a no-op. The redundant clear in cgocallbackg is
removed.
As a defensive measure, appendLocsForStack returns locs instead of
panicking if len(l.pcs) > len(stk), so any future similar race
degrades gracefully.
TestIssue70529 in proto_test.go reproduces the panic deterministically
by feeding profileBuilder a long-stack sample followed by a
short-stack sample. TestCgoCallbackPprofRace exercises the scenario
with real C-to-Go callbacks under continuous CPU profiling.
Updates golang#70529
|
This PR (HEAD: 92154ae) has been imported to Gerrit for code review. Please visit Gerrit at https://go-review.googlesource.com/c/go/+/815820. Important tips:
|
|
Message from 李兆龙: Patch Set 3: (1 comment) Please don’t reply on this GitHub thread. Visit golang.org/cl/815820. |
2 participants
sigprofNonGoPC produces a fixed-length 2 stack {pc, _ExternalCode}
when a SIGPROF lands in the window between exitsyscall() returning
and m.isExtraInC being cleared in cgocallbackg. If that PC was
previously sampled in a normal Go context, appendLocsForStack has
cached an inlined expansion (l.pcs) longer than the short stack, and
panics with "stack too short to match cached location".
The panic fires in the runtime-spawned profileWriter goroutine, which
user code cannot recover. In -buildmode=c-shared binaries the runtime
forces tracebackCrash, so it becomes SIGABRT, crashing the host
process. This is statistically inevitable under sustained cgo
callbacks with continuous CPU profiling.
Fix: rather than panicking, drop the remaining unmatchable PCs of the
sample. The leaf Location was already recorded, so the sample keeps a
correct leaf frame at the cost of a few missing synthetic _ExternalCode
caller frames on rare samples.
TestIssue70529 reproduces the panic deterministically by feeding
profileBuilder a long-stack sample followed by a short-stack sample.
Without the fix it panics; with the fix it succeeds.
Updates #70529